Use subclasses of Net::HTTPRequest instead of Net::HTTPGenericRequest #647
Conversation
|
@iMacTia do you want me to provide a proof-of-concept setup that demonstrates the problem? |
|
Hi @fotos.
This put us in a situation where we should address the issue on our side, as they're pretty clear on theirs. Still, I'm a bit concerned about the issue with Ruby < 2.2 compatibility as we definitely want to keep supporting 1.9.3+.
To be honest I don't fully support neither of them. The former seems dangerous (we're removing a test that could prove itself useful), while the latter seems over-complicated. If I really have to pick one, I would probably go for the latter to have the best test-coverage, accompanied with a descriptive comment on why we're doing that. As a sidenote, what can you tell me about the other error?
|
Instead of directly instantiating Net::HTTPGenericRequest, use the appropriate Net::HTTPRequest subclass. Fixes a problem with net-http-persistent adapter that is checking the subclasses of Net::HTTPRequest for idempotence support.
fotos
force-pushed
the
fix-net-http-persistent-initialization
branch
from
eb27d01 to
90bdb60
Compare
Fixes a problem with
net-http-persistentadapter that is checking the subclasses ofNet::HTTPRequestfor idempotence support.Instead of directly instantiating
Net::HTTPGenericRequest, use the appropriateNet::HTTPRequestsubclass.We stumbled upon this situation at @catawiki when we switched our internal API client to
faraday(backed bynet-http-persistent). Whenever we reloaded ournginxinstances we sawNet::HTTP::Persistent::Error: too many connection resets (due to end of file reached - EOFError)exception being thrown to the application level, due to the sockets being closed.It took a lot of trial and error and quite some digging to figure out what's going on even tho we haven't changed the underlying http library (
net-http-persistent). There are a couple of reports online that might to be related to this issue (sparklemotion/mechanize#123, drbrain/net-http-persistent#37) and a couple of gems such aselasticsearch-rubymight be affected by this issue depending on the configuration.The problem is that
faradayis creating a class ofNet::HTTPGenericRequestfor thenet-httpadapter. Yet thenet-http-persistentgem (whosefaradayadapter implementation is subclassing thenet-httpadapter) is expecting a Net::HTTPRequest subclass. Based on the class it decides whether a request is idempotent.If a request is idempotent then it automatically retries the request once.
As a temporary workaround we used the
Faraday::Request::Retrymiddleware as follows:(
maxis set intentionally to1)The idea for the approach in this PR is liberally taken from mechanize.
Note: this change is breaking the specs. In Ruby 1.8, 1.9.3, 2.1 the
Net::Http::Optionsis marked as not returning a body. This has been fixed in Ruby 2.2, even tho OPTIONS may have a request body since RFC2616. Previously the specs passed because the check was:head != env[:method]. So I'm not sure what's the appropriate approach for the specs or if this change is acceptable overall (the behaviour now depends on the Ruby version?).I'll openI've opened PR #83 tonet-http-persistentas well to use#methodinstead of checking the class hierarchy.@mislav what do you think?